Repository navigation
Fix the ActionMailer error handlers in delivery jobs, and log a failed email once - #21
Merged
Merged
Conversation
ActionMailer::MailDeliveryJob passes a delivery error to the mailer class (handle_exception), and Rails looks the rescue_from handler up on the class. LogStruct's handlers (log_and_reraise_error, and the log_and_ignore_error / log_and_report_error that apps use in their own rescue_from lines) were only instance methods, so every email that failed to send in a delivery job raised "NameError: undefined method 'log_and_reraise_error' for class", with the real error as its cause. Define them on the class as well. An error the mailer's own handler already logged and re-raised is marked, so the class handler re-raises it without logging or reporting it twice.
A MailDeliveryJob's arguments include the mail's own arguments, which can be secrets with no key to filter on: Devise passes its confirmation, password reset and invitation tokens to the mailer, and deliver_later puts them in the job. ActiveJob lines logged them on enqueue, start and finish.
The mailer's handler logged a failed delivery twice: its structured line, then the same struct again before reporting. In Log and Report modes handle_exception logged it a third time. In Raise mode (the default for standard errors) handle_exception raised before the error was marked as logged, so when it reached the mailer class (MailDeliveryJob) the class handler logged it again. handle_exception takes log: false for callers that have already logged the error, and the mailer marks the error as logged straight after its structured line.
In Raise mode (the default for standard errors), a mailer whose rescue_from calls log_and_report_error has handle_exception raise the error. In a delivery job the error then reached the mailer class's handler, which saw that it was already logged and returned without raising, so the job succeeded and the error was never reported. The class handler now raises an error the mailer already logged, as deliver_now does, without logging it again. The marks use LogStruct::LoggedErrors instead of an instance variable on the error. The tests now give each test its own copy of the whole configuration. The code examples left string scrubbers, lograge options, salts, filter settings and the LogStruct error handling mode set for later tests. The docs say the mail argument filtering applies while the ActiveJob integration is on.
|
Warning Review limit reached
This review includes 9 billable files and costs up to $2.25.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 37 minutes for your next included review. View limit details
Comment |
This was referenced Oct 17, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sixth of twelve stacked PRs for 0.2.0.
What this fixes
ActionMailer::MailDeliveryJobraisedNameError(undefined method 'log_and_reraise_error' for class) instead of the real error. The job passes the error to the mailer class, and the handlers were only defined on instances. They're now defined on the class too.deliver_lateras plain strings, which key-based filtering can't find. ForMailDeliveryJob, job lines now log only the mailer, the mail and the delivery method asarguments. Other jobs' arguments are logged as before (when the job'slog_argumentsis on).Raisemode (the default for standard errors),log_and_report_errorraises the error. In a delivery job it then reached the mailer class's handler, which saw that it was already logged and returned, so the job succeeded with no retry and no report. The class handler now raises an error the mailer already logged, asdeliver_nowwould, without logging it again. An error the class handler handles itself isn't marked, so a mailer given the same error object again still handles it.LogStruct.handle_exceptiontakeslog:(defaulttrue), for callers whose own line already logged the error.error_reporting_handlerset for later tests, so the mailer tests failed when one ran first.Testing
Every regression test fails without its fix. Sorbet and the unit suite (308 tests) pass at this branch's tip.
Stack for 0.2.0, merged in order: #16 → #17 → #18 → #19 → #20 → #21 → #22 → #23 → #24 → #25 → #26 → #27